Skip to content

feat(cli): layered DATABASE_URL resolution for db / schema commands#377

Merged
coderdan merged 8 commits intomainfrom
dan/database-url-resolution
May 1, 2026
Merged

feat(cli): layered DATABASE_URL resolution for db / schema commands#377
coderdan merged 8 commits intomainfrom
dan/database-url-resolution

Conversation

@coderdan
Copy link
Copy Markdown
Contributor

@coderdan coderdan commented Apr 30, 2026

Closes #376. Stacks on top of #375 (jiti default-export unwrap fix) — both together deliver the full onboarding experience.

Problem

Running any DB-touching command without DATABASE_URL exported in the shell or in a loaded .env* file fails with the cryptic Zod error:

Error: Invalid stash.config.ts
  - databaseUrl: Invalid input: expected nonoptional, received undefined

Users provide DB URLs through many channels: --database-url flag, shell exports, mise.toml, direnv, dotenv-cli, local Supabase, or simply "let me paste it once". The CLI handled only the dotenv path.

Fix

The scaffolded stash.config.ts now calls the resolver directly:

import { defineConfig, resolveDatabaseUrl } from '@cipherstash/cli'

export default defineConfig({
  databaseUrl: await resolveDatabaseUrl(),
  client: './src/encryption/index.ts',
})

resolveDatabaseUrl() walks sources in order; first hit wins:

  1. --database-url <url> flag (explicit override)
  2. process.env.DATABASE_URL (shell, mise, direnv, dotenv files already loaded by bin/stash.ts)
  3. supabase status --output envDB_URL, gated on --supabase or a supabase/config.toml in cwd
  4. Interactive p.text prompt — skipped under CI=true or non-TTY stdin
  5. Hard fail with a source-naming error message

CLI flag values are threaded into the user's config evaluation via an AsyncLocalStorage scope (withResolverContext) wrapped around the jiti import in loadStashConfig. process.env.DATABASE_URL is never mutated by this code. The connection string is never written to disk — stash.config.ts references the resolver, not a literal. Concurrent loadStashConfig calls each get their own ALS context (regression test included).

The source is logged (Using DATABASE_URL from --database-url flag / from supabase status / from prompt); the URL itself is never echoed to stdout.

After a prompt-sourced run, a p.note nudges the user to set DATABASE_URL in their existing dotenv file (detected from cwd, defaults to .env) so they don't get re-prompted next time.

Surface

--database-url <url> accepted on all seven DB-touching commands: db install, db push, db upgrade, db status, db validate, db test-connection, schema build. HELP text updated.

init/build-schema is not wired through the resolver — it reads process.env.DATABASE_URL for provider hints pre-config and should not trigger the prompt flow.

Test plan

  • pnpm --filter @cipherstash/cli test — 117 unit tests pass (+ new __tests__/database-url.test.ts covering each source, CI guard, concurrent ALS isolation, dotenv-file detection)
  • pnpm --filter @cipherstash/cli test:e2e — 10 E2E tests pass (+ new tests/e2e/database-url.e2e.test.ts: flag-source surfaces the label, CI guard exits 1 with the CI message)
  • biome check clean on changed files
  • pnpm --filter @cipherstash/cli build clean

Manual verification

Verified each resolution path locally against the built dist/bin/stash.js with a temp stash.config.ts that calls await resolveDatabaseUrl():

  • Flag overridedb test-connection --database-url <url> logs Using DATABASE_URL from --database-url flag, then attempts connection. URL never echoed in output.
  • Env sourceDATABASE_URL=<url> db test-connection runs silently (no source label, env is the default).
  • CI guardCI=true db test-connection (no env, no flag) exits 1 with the CI-specific message; never prompts.
  • Interactive promptdb test-connection (no env, no flag) shows the alternatives tip via p.note, then the URL prompt.
  • Prompt + hint file detection — with .env.local present, the post-prompt nudge says Set DATABASE_URL in .env.local…; with no dotenv file, defaults to .env. Tip note also matches the detected file.
  • Malformed flag--database-url not-a-url exits 1 with Invalid --database-url.
  • Cancel the prompt — Ctrl-C at the URL prompt exits 0 with Cancelled.
  • Supabase fallback — against a real supabase init && supabase start project, the resolver shells out to supabase status --output env, parses DB_URL, logs Using DATABASE_URL from supabase status, and connects successfully.
  • Connection-failure hint — when the resolved URL fails to connect, the message points users at --database-url / env / the detected dotenv file, not at stash.config.ts.
  • process.env.DATABASE_URL is unchanged after every scenario — the resolver does not mutate the env.
  • The URL value never appears in any log line — only source labels (from --database-url flag, from supabase status, from prompt).

Note on coverage

The unit tests use vi.mock to stub node:child_process, @clack/prompts, and detectSupabaseProject, so each source path is exercised in isolation. The E2E tests run the built dist/bin/stash.js through the pty harness against a temp stash.config.ts for the flag and CI-guard cases. The non-TTY path is covered by the unit suite (Object.defineProperty(process.stdin, 'isTTY', false)) since the pty harness always provides a TTY.

The ALS instance is parked on globalThis under a Symbol.for key — necessary because tsup ships the library (dist/index.js, what user configs import) and the binary (dist/bin/stash.js, what npx runs) as separate bundles, each with its own copy of database-url.ts. The Symbol.for rendezvous gives both bundles a single shared ALS for the lifetime of the process.

Summary by CodeRabbit

  • New Features

    • Added a global --database-url flag for DB and schema commands.
    • Exposed resolveDatabaseUrl and its options from the CLI API.
  • Improvements

    • Layered DATABASE_URL resolution (flag → env → Supabase → interactive), no env mutation, CI/non‑TTY guards, dotenv-file hints, clearer source-aware messages.
    • Config loading accepts resolver options; scaffold templates use resolveDatabaseUrl().
    • CLI help/intros now reflect detected package manager.
  • Tests

    • New unit and E2E tests covering precedence, CI behavior, Supabase discovery, prompt UX, cancellation, and concurrent isolation.
  • Docs

    • Updated contributor guidance to require changesets for public-surface changes.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 30, 2026

🦋 Changeset detected

Latest commit: cb31fb3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@cipherstash/cli Minor
@cipherstash/e2e Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an AsyncLocalStorage-backed resolver and CLI --database-url flag; threads flag into db/schema commands and config loading; implements layered resolution (flag → env → Supabase → interactive prompt) with CI/TTY guards, new messages, unit and E2E tests, and scaffold/template updates.

Changes

Cohort / File(s) Summary
Core Resolver
packages/cli/src/config/database-url.ts
New ALS-backed withResolverContext, resolveDatabaseUrl, detectDotenvFile, and ResolveDatabaseUrlOptions; resolves flag → env → supabase → prompt; validates, logs source, handles CI/TTY and exit codes.
Public API
packages/cli/src/index.ts
Exports resolveDatabaseUrl and ResolveDatabaseUrlOptions.
CLI Entry & Flag wiring
packages/cli/src/bin/stash.ts
Adds global --database-url flag and threads databaseUrl into db/schema command invocations.
Config loader integration
packages/cli/src/config/index.ts
loadStashConfig accepts resolver options and evaluates config inside withResolverContext so in-config resolveDatabaseUrl() observes CLI-context.
DB Commands
packages/cli/src/commands/db/...
install.ts, push.ts, status.ts, test-connection.ts, upgrade.ts, validate.ts, config-scaffold.ts
Command signatures accept optional databaseUrl and forward it to loadStashConfig as databaseUrlFlag; scaffold now uses await resolveDatabaseUrl(); runnerCommand/detectPackageManager used for messages.
Schema Command
packages/cli/src/commands/schema/build.ts
builderCommand accepts databaseUrl and forwards it into loadStashConfig; minor formatting tweaks.
Messages
packages/cli/src/messages.ts
Adds messages.db entries: source labels, prompt text/tip, validation/malformed/CI vs interactive missing messages, connection hint and url hint helper.
Unit tests
packages/cli/src/__tests__/database-url.test.ts
New Vitest suite covering precedence (flag/env/supabase/prompt), Supabase parsing/fallbacks, CI/TTY behavior, exit codes, prompt tips, and concurrency isolation via withResolverContext.
E2E tests
packages/cli/tests/e2e/database-url.e2e.test.ts
New E2E tests spawning CLI with temp stash.config.ts to assert flag resolution, source-label logs, and CI missing-variable behavior.
Changeset & Docs
.changeset/cli-database-url-resolution.md, AGENTS.md
Adds changeset describing behavior and updates contributor guidance for changesets.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI
    participant Resolver
    participant Env
    participant Supabase
    participant Prompt
    participant Config

    User->>CLI: run db/schema command (maybe `--database-url`)
    CLI->>Resolver: withResolverContext(resolveDatabaseUrl(opts))
    alt databaseUrlFlag present
        Resolver->>Resolver: validate flag URL
        Resolver-->>CLI: return URL (source: flag)
    else process.env.DATABASE_URL present
        Resolver->>Env: read DATABASE_URL
        Resolver-->>CLI: return URL (source: env)
    else supabase allowed/detected
        Resolver->>Supabase: run `supabase status --output env` (or fallback)
        alt DB_URL found
            Resolver-->>CLI: return URL (source: supabase)
        else no DB_URL / error
            alt interactive (TTY && !CI)
                Resolver->>Prompt: ask for URL (show dotenv tip)
                Prompt-->>Resolver: url / cancel
                alt url provided
                    Resolver-->>CLI: return URL (source: prompt)
                else canceled
                    Resolver-->>CLI: exit(0)
                end
            else
                Resolver-->>CLI: exit(1)
            end
        end
    end
    CLI->>Config: load stash.config.ts inside withResolverContext
    Config->>Resolver: config may call resolveDatabaseUrl() and use resolved URL
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • auxesis
  • calvinbrewer

Poem

🐇 I sniffed the flag, the env, the supabase trail,
I asked the prompt when the others all failed.
Context kept tidy, no globals to mar—
Config loads clean, and I hop to the bar. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically summarizes the main feature being added: a layered DATABASE_URL resolution system for db and schema commands.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dan/database-url-resolution

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/cli/src/commands/db/test-connection.ts (1)

48-54: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the failure hint to include the new URL sources.

After this change, connection failures can come from --database-url, the env var, or Supabase discovery. Pointing users only at stash.config.ts / .env is now misleading.

💡 Suggested fix
-    p.log.info('Check your databaseUrl in stash.config.ts or .env file.')
+    p.log.info(
+      'Check the resolved DATABASE_URL source (--database-url, .env, or Supabase discovery).',
+    )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/db/test-connection.ts` around lines 48 - 54, Update
the failure hint printed in test-connection.ts so it reflects all possible URL
sources instead of only stash.config.ts/.env: modify the p.log.info call in the
error branch (the block that constructs message and calls p.log.error) to
mention --database-url, the environment variable, and Supabase discovery as
places to check; locate the error-handling block that defines message and calls
p.log.error / p.log.info and change the info text accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cli/src/config/database-url.ts`:
- Around line 160-161: The CI detection currently only checks process.env.CI ===
'true', causing CI values like '1' or other truthy strings to be missed; update
the isCi computation (used alongside isInteractive) to treat common truthy CI
values case-insensitively (e.g., '1', 'true', 'yes') or simply check for a
non-empty process.env.CI value (e.g., Boolean(process.env.CI) or a regex match)
so that isInteractive (which uses process.stdin.isTTY && !isCi) behaves
correctly in CI environments.

---

Outside diff comments:
In `@packages/cli/src/commands/db/test-connection.ts`:
- Around line 48-54: Update the failure hint printed in test-connection.ts so it
reflects all possible URL sources instead of only stash.config.ts/.env: modify
the p.log.info call in the error branch (the block that constructs message and
calls p.log.error) to mention --database-url, the environment variable, and
Supabase discovery as places to check; locate the error-handling block that
defines message and calls p.log.error / p.log.info and change the info text
accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7896bf05-8179-4618-9876-ae1f3978a98b

📥 Commits

Reviewing files that changed from the base of the PR and between 3d12510 and d7bd47c.

📒 Files selected for processing (12)
  • packages/cli/src/__tests__/database-url.test.ts
  • packages/cli/src/bin/stash.ts
  • packages/cli/src/commands/db/install.ts
  • packages/cli/src/commands/db/push.ts
  • packages/cli/src/commands/db/status.ts
  • packages/cli/src/commands/db/test-connection.ts
  • packages/cli/src/commands/db/upgrade.ts
  • packages/cli/src/commands/db/validate.ts
  • packages/cli/src/commands/schema/build.ts
  • packages/cli/src/config/database-url.ts
  • packages/cli/src/messages.ts
  • packages/cli/tests/e2e/database-url.e2e.test.ts

Comment thread packages/cli/src/config/database-url.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a layered DATABASE_URL resolver to improve onboarding for DB-touching CLI commands by supporting multiple sources (flag/env/Supabase/prompt) before stash.config.ts is loaded, plus tests to cover the new resolution paths.

Changes:

  • Introduce resolveDatabaseUrl() and wire it into DB/schema commands, including a new --database-url flag.
  • Add user-facing message handles for resolver output and failure modes.
  • Add unit + E2E coverage for resolver behavior (flag + CI guard).

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/cli/src/config/database-url.ts New layered resolver for DATABASE_URL, including Supabase and prompt fallbacks.
packages/cli/src/bin/stash.ts Plumbs --database-url into db/schema commands and updates help text.
packages/cli/src/messages.ts Adds message handles for resolver logging/prompts/errors.
packages/cli/src/commands/db/install.ts Runs resolver before config load; accepts databaseUrl option.
packages/cli/src/commands/db/upgrade.ts Runs resolver before config load; accepts databaseUrl option.
packages/cli/src/commands/db/validate.ts Runs resolver before config load; accepts databaseUrl option.
packages/cli/src/commands/db/status.ts Runs resolver before config load; accepts databaseUrl option.
packages/cli/src/commands/db/test-connection.ts Runs resolver before config load; accepts databaseUrl option.
packages/cli/src/commands/db/push.ts Adds databaseUrl option and calls resolver before config load.
packages/cli/src/commands/schema/build.ts Runs resolver before config load; accepts databaseUrl option.
packages/cli/src/tests/database-url.test.ts New unit tests for resolver source ordering and guards.
packages/cli/tests/e2e/database-url.e2e.test.ts New E2E tests validating flag path + CI fail-fast behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/cli/src/commands/db/push.ts Outdated
Onboarding users running `stash db install` (or any of the six other
DB-touching commands) without an exported `DATABASE_URL` would hit a
cryptic "Invalid stash.config.ts: databaseUrl received undefined"
error from Zod. The CLI already loads `.env.local` / `.env`* via
dotenv, but had no story for `--database-url` flags, local Supabase,
or a pasted-once one-off value.

Adds `src/config/database-url.ts` exporting `resolveDatabaseUrl()` —
called at the top of every DB-touching command before
`loadStashConfig`. It walks sources in order:

  1. `--database-url <url>` flag (explicit override)
  2. `process.env.DATABASE_URL` (shell, mise, direnv, dotenv files)
  3. `supabase status --output env` -> DB_URL, gated on `--supabase`
     or a `supabase/config.toml` in cwd
  4. Interactive `p.text` prompt (skipped under `CI=true` or
     non-TTY stdin)
  5. Hard fail with a source-naming error message

The resolved URL is mutated into `process.env.DATABASE_URL` (only
when env was unset/empty, or the source was an explicit flag) so the
existing scaffolded `process.env.DATABASE_URL!` reference in
`stash.config.ts` resolves transparently. The connection string is
never written to disk — `stash.config.ts` keeps its declarative env
reference. The source label is logged (`Using DATABASE_URL from
--database-url flag` / `from supabase status` / `from prompt`) but
the URL itself is never echoed.

Wired through `bin/stash.ts` argv (the parser already supports
`--key value`), HELP text updated. All seven DB-touching commands
(`db install`, `db push`, `db upgrade`, `db status`, `db validate`,
`db test-connection`, `schema build`) accept `--database-url` and
call the resolver before `loadStashConfig`.

After a prompt-sourced run, a `p.note` nudges the user to set
DATABASE_URL in the existing dotenv file (or `.env` if none exists)
so they don't get re-prompted next time.

Tests:
- 13 unit tests (`__tests__/database-url.test.ts`) covering each
  source, malformed flag, empty-string-env fallthrough, supabase
  fallthrough on missing binary / no-DB_URL output / no project,
  prompt-cancel, CI guard, non-TTY guard, hint-file detection.
- 2 E2E tests (`tests/e2e/database-url.e2e.test.ts`) driving the
  built binary through the pty harness: `--database-url` flag
  surfaces the source label and proceeds to (failing) connection,
  CI=true with no creds exits 1 with the CI message.

`init`/build-schema is intentionally NOT wired through the resolver:
it reads `process.env.DATABASE_URL` for provider hints pre-config and
should not trigger the prompt flow.

Note: stacks on top of #375 (the jiti default-export unwrap fix). The
resolver populates env, but `loadStashConfig` still needs the jiti
fix to read the resulting config correctly. Both PRs together
deliver the full onboarding experience.
@coderdan coderdan force-pushed the dan/database-url-resolution branch from d7bd47c to 35d9a2c Compare May 1, 2026 01:11
…utation

The previous design had the resolver mutate `process.env.DATABASE_URL`
in-flight before `loadStashConfig`, with the scaffolded
`stash.config.ts` referencing `process.env.DATABASE_URL!`. Two issues:

  1. Reading the config file made it look like the URL only ever came
     from env, even when the user had passed `--database-url` or hit
     the supabase-status fallback. Misleading.
  2. Mutating `process.env` in-flight is a global side-effect.

New shape — the scaffolded config calls the resolver directly:

  import { defineConfig, resolveDatabaseUrl } from '@cipherstash/cli'
  export default defineConfig({
    databaseUrl: await resolveDatabaseUrl(),
  })

Reading the file tells you exactly how the URL is resolved. No env
side-effects.

Implementation:

- `withResolverContext({ databaseUrlFlag, supabase }, fn)` runs `fn`
  inside an `AsyncLocalStorage` scope. Any `resolveDatabaseUrl()`
  descendant in the async-flow reads the options via `als.getStore()`.
  Each `loadStashConfig` call gets its own scope, so concurrent loads
  don't step on each other (verified by a dedicated unit test).

- The ALS instance is stashed on `globalThis` under a
  `Symbol.for`-keyed slot. Necessary because tsup ships the library
  (`dist/index.js`) and the binary (`dist/bin/stash.js`) as separate
  bundles, each with its own copy of `database-url.ts`. A bare
  module-level ALS would produce two independent stores: the CLI sets
  context on the binary's instance, the user's config (loaded via
  jiti from inside the binary process) reads from the library's. The
  globalThis rendezvous gives both bundles a single shared instance.

- `resolveDatabaseUrl` returns `string` (was previously
  `{ url, source }`) so the scaffolded `databaseUrl: await
  resolveDatabaseUrl()` is direct. The source label is logged
  internally; not part of the return contract.

- All seven DB-touching commands stop calling `resolveDatabaseUrl`
  themselves. Instead they pass `{ databaseUrlFlag, supabase }` to
  `loadStashConfig`, which threads them into the ALS scope around the
  jiti-import.

- Prompt now opens with a `p.note` listing the alternative paths
  (--database-url flag, env var, .env file) so users aren't stuck in
  an interactive flow when a flag would do.

- `process.env.DATABASE_URL` is never mutated by this code.

Tests:

- Unit suite (16 tests, was 13) updated for the new return type and
  no-env-mutation invariant. Adds an explicit concurrency test:
  two `withResolverContext` scopes started in parallel each see their
  own flag value. This is the regression net for the ALS choice.

- E2E suite updated: the test fixture config imports
  `resolveDatabaseUrl` from an absolute path to `dist/index.js` (the
  tmp dir has no node_modules). Real users get a clean
  `import { resolveDatabaseUrl } from '@cipherstash/cli'`.

No legacy-config handling — this hasn't shipped yet, fresh installs
get the new template directly.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cli/src/__tests__/database-url.test.ts`:
- Around line 63-81: The test teardown uses vi.clearAllMocks() which does not
restore spied globals like process.exit, causing spy leakage; replace
vi.clearAllMocks() with vi.restoreAllMocks() in the afterEach block so spies
created in tests (e.g., process.exit spies referenced around the tests that
create spies at lines near 105,127,159,176,190,234,249,264) are fully restored
to their original implementations and won't affect later tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b887a2f8-bbc7-440e-83de-257f2c5db27a

📥 Commits

Reviewing files that changed from the base of the PR and between 35d9a2c and d4f2ec5.

📒 Files selected for processing (14)
  • packages/cli/src/__tests__/database-url.test.ts
  • packages/cli/src/commands/db/config-scaffold.ts
  • packages/cli/src/commands/db/install.ts
  • packages/cli/src/commands/db/push.ts
  • packages/cli/src/commands/db/status.ts
  • packages/cli/src/commands/db/test-connection.ts
  • packages/cli/src/commands/db/upgrade.ts
  • packages/cli/src/commands/db/validate.ts
  • packages/cli/src/commands/schema/build.ts
  • packages/cli/src/config/database-url.ts
  • packages/cli/src/config/index.ts
  • packages/cli/src/index.ts
  • packages/cli/src/messages.ts
  • packages/cli/tests/e2e/database-url.e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/cli/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/cli/src/commands/db/push.ts
  • packages/cli/src/commands/db/validate.ts
  • packages/cli/src/messages.ts

Comment thread packages/cli/src/__tests__/database-url.test.ts
Comment thread packages/cli/src/config/database-url.ts
coderdan added 5 commits May 1, 2026 12:56
…v file

Two follow-ups from manual testing:

1. The prompt tip referred to ".env file" generically, even when the
   project had a `.env.local` (the post-resolution hint already
   matched the detected file, but the up-front tip didn't).

2. `db test-connection`'s connection-failure handler suggested
   "Check your databaseUrl in stash.config.ts or .env file". With
   the new design `stash.config.ts` doesn't carry the URL — it
   calls `resolveDatabaseUrl()` — so pointing users there is
   actively misleading.

Both messages now go through `detectDotenvFile(cwd)` (newly
exported) and the message factories
`messages.db.urlPromptTip(file)` and
`messages.db.urlConnectionFailedHint(file)`. The user sees the
exact dotenv file they have in their project, plus the other ways
they can specify the URL (--database-url flag, env var).

Adds a unit test covering the dynamic-tip behaviour for both the
detected-file and default-`.env` cases.
Two findings:

1. CI detection in resolveDatabaseUrl was only matching `CI === 'true'`
   exactly. Some CI providers set `CI=1` or use mixed case. Broadened
   to `/^(1|true)$/i` against trimmed input, plus a parameterized test
   covering 'true' / 'TRUE' / '1' / ' true '.

2. Test afterEach used vi.clearAllMocks(), which resets mock-call
   history but does NOT restore spies on global objects. The
   process.exit spies created in individual tests would leak across
   tests (each test re-spied so the symptom was masked, but the leak
   was real). Switched to vi.restoreAllMocks(), which calls
   mockRestore() on all spies and reverts them to their originals.
The connection-failure hint update (commit 8248200) routed
testConnectionCommand's `p.log.info` through
`messages.db.urlConnectionFailedHint(detectDotenvFile())` so users
see flag / env / dotenv-file as fixable sources, not the
now-irrelevant `stash.config.ts`. Behaviour was manually verified
but no automated test guarded it.

Extends the existing `--database-url` flag E2E case (where the
connection deliberately fails against a bogus host:port) with two
assertions:

  - the failure output contains the new hint, with the dotenv file
    detected from the tmp dir (defaults to `.env` since the dir
    has no dotenv files)
  - the OLD misleading "stash.config.ts or .env file" wording is
    absent

Same spawn, two extra `expect()` calls. Closes the gap surfaced by
the manual-vs-automated audit on PR #377.
Adds step 7 to the "Adding Features Safely" checklist explicitly
requiring a changeset when a change affects a published package's
public behaviour. Includes the exact file format, points at the
changeset-bot warning, and lists the carve-outs (test-only PRs,
internal refactors, tooling) so contributors don't add empty
changesets when one isn't needed.

Without this, agent-driven PRs were silently shipping changes that
didn't surface in CHANGELOG.md or trigger version bumps.
Copy link
Copy Markdown
Contributor

@auxesis auxesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really solid work that you absolutely love to see.

This config lookup mechanism is really robust.

Have left some comments about hard coded npx that need to be fixed up before merge. Some are introduced in this PR, others are already there. #379 should make this much easier to fix.

Comment thread .changeset/cli-database-url-resolution.md
Comment thread packages/cli/src/commands/db/status.ts Outdated
Comment thread packages/cli/src/commands/db/upgrade.ts Outdated
Comment thread packages/cli/src/commands/db/validate.ts Outdated
Lindsay flagged on PR #377 that the DB commands hard-code `npx
@cipherstash/cli ...` in their `p.intro` and warning messages —
some lines I touched in this PR, others pre-existing. #379 already
landed `runnerCommand(pm, ref)` + `detectPackageManager()` for
exactly this; sweeping the six DB commands to use it now.

A user who runs `bunx @cipherstash/cli db status` now sees
`bunx @cipherstash/cli db status` as the intro banner (was: always
`npx ...` regardless of how they invoked the CLI), and warnings
like "Run `bunx @cipherstash/cli db install` to install it" or
"run `pnpm dlx @cipherstash/cli db push` to create it" follow the
same rule.

Files touched (all already in this PR's diff for the resolver
plumbing):

- db/install.ts — intro
- db/upgrade.ts — intro + "run db install" warning
- db/push.ts — intro
- db/status.ts — intro + 2 warnings (db install, db push)
- db/validate.ts — intro
- db/test-connection.ts — intro

Out of scope: the help-banner usage examples in `bin/stash.ts` and
`auth/index.ts`. Those are documentation listing all supported
commands, not runtime "this is what I ran" labels — separate UX
question whether `--help` should pick a single runner or list all
four. The wizard reference in `db/install.ts:276` is a comment
inside a generated SQL file's header; same separate concern.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.changeset/cli-database-url-resolution.md:
- Around line 9-12: The fenced code block showing "Error: Invalid
stash.config.ts" in .changeset/cli-database-url-resolution.md is missing a
language specifier causing MD040; update that fenced block by adding a language
identifier (e.g., "text") after the opening triple backticks so the block
becomes ```text and the linter rule fenced-code-language is satisfied.

In `@AGENTS.md`:
- Line 175: The fenced code block shown with triple backticks is missing a
language tag (causing MD040); update that fence to include a language identifier
(for example use "text") so the opening delimiter becomes ```text and the block
remains otherwise unchanged—look for the ``` fence in AGENTS.md and add the
language tag.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 300c785c-a07b-4769-b9a8-c3b411ee9167

📥 Commits

Reviewing files that changed from the base of the PR and between d46575d and cb31fb3.

📒 Files selected for processing (8)
  • .changeset/cli-database-url-resolution.md
  • AGENTS.md
  • packages/cli/src/commands/db/install.ts
  • packages/cli/src/commands/db/push.ts
  • packages/cli/src/commands/db/status.ts
  • packages/cli/src/commands/db/test-connection.ts
  • packages/cli/src/commands/db/upgrade.ts
  • packages/cli/src/commands/db/validate.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/cli/src/commands/db/validate.ts
  • packages/cli/src/commands/db/push.ts
  • packages/cli/src/commands/db/test-connection.ts
  • packages/cli/src/commands/db/upgrade.ts

Comment thread .changeset/cli-database-url-resolution.md
Comment thread AGENTS.md
@coderdan coderdan merged commit 2336062 into main May 1, 2026
7 checks passed
@coderdan coderdan deleted the dan/database-url-resolution branch May 1, 2026 05:51
coderdan added a commit that referenced this pull request May 1, 2026
Follow-up to PR #377 addressing Lindsay's feedback that the HELP
banners and the wizard reference in `db install`'s "Next steps"
panel still hard-coded `npx`. #379 already added
`runnerCommand(pm, ref)` + `detectPackageManager()`; this sweep
applies it to the remaining places.

User-visible: a user who runs `bunx @cipherstash/cli --help` now
sees `bunx @cipherstash/cli` in the Usage line and Examples, and
the wizard suggestion at the end of `db install` matches the user's
runner. Same for `pnpm dlx`, `yarn dlx`, default `npx`.

Touches:
- `bin/stash.ts` — `HELP` template now interpolates a `STASH`
  constant (`runnerCommand(PM, '@cipherstash/cli')`) for the
  Usage line, the six Examples, and the `requireStack` hint.
- `commands/auth/index.ts` — same pattern for the auth HELP, with
  a `STASH_AUTH` constant for the Usage line + two examples.
- `commands/db/install.ts` — the wizard line in `printNextSteps`
  uses `runnerCommand(pm, '@cipherstash/wizard')`. Also brings
  the `p.intro('npx @cipherstash/cli db install')` line in line
  with the runner-aware pattern (was already pinned for the same
  reason in PR #377; this branch is off main, so it lands here too).

Messages: `messages.cli.usagePrefix` and `messages.auth.usagePrefix`
become stable leaders (`'Usage: '`) — the runner+package suffix
is appended at render time. E2E assertions stay runner-agnostic
because they `toContain('Usage: ')` rather than the full prefix.

Out of scope (still): the migrate stub message in
`messages.db.migrateNotImplemented` keeps its hard-coded `npx`
form. The stub is a placeholder and the inconsistency only shows
when someone runs a not-yet-implemented command — not worth
plumbing for.
coderdan added a commit that referenced this pull request May 4, 2026
Follow-up to PR #377 addressing Lindsay's feedback that the HELP
banners and the wizard reference in `db install`'s "Next steps"
panel still hard-coded `npx`. #379 already added
`runnerCommand(pm, ref)` + `detectPackageManager()`; this sweep
applies it to the remaining places.

User-visible: a user who runs `bunx @cipherstash/cli --help` now
sees `bunx @cipherstash/cli` in the Usage line and Examples, and
the wizard suggestion at the end of `db install` matches the user's
runner. Same for `pnpm dlx`, `yarn dlx`, default `npx`.

Touches:
- `bin/stash.ts` — `HELP` template now interpolates a `STASH`
  constant (`runnerCommand(PM, '@cipherstash/cli')`) for the
  Usage line, the six Examples, and the `requireStack` hint.
- `commands/auth/index.ts` — same pattern for the auth HELP, with
  a `STASH_AUTH` constant for the Usage line + two examples.
- `commands/db/install.ts` — the wizard line in `printNextSteps`
  uses `runnerCommand(pm, '@cipherstash/wizard')`. Also brings
  the `p.intro('npx @cipherstash/cli db install')` line in line
  with the runner-aware pattern (was already pinned for the same
  reason in PR #377; this branch is off main, so it lands here too).

Messages: `messages.cli.usagePrefix` and `messages.auth.usagePrefix`
become stable leaders (`'Usage: '`) — the runner+package suffix
is appended at render time. E2E assertions stay runner-agnostic
because they `toContain('Usage: ')` rather than the full prefix.

Out of scope (still): the migrate stub message in
`messages.db.migrateNotImplemented` keeps its hard-coded `npx`
form. The stub is a placeholder and the inconsistency only shows
when someone runs a not-yet-implemented command — not worth
plumbing for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stash db commands fail with cryptic error when DATABASE_URL is not exported

3 participants